-
Notifications
You must be signed in to change notification settings - Fork 424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert pen input flow and map pen to touch on mobile platforms #6523
base: master
Are you sure you want to change the base?
Conversation
Is this a direct revert? Are you intending to make a PR to re-apply the changes for future consideration (so they aren't lost to the void)? |
I can make a PR but the best state it could be in is a draft and eventually become outdated structure. The pen input implementation has shown to be not working very well with touch input, and I believe a major refactor in how it works is required rather than reviving it back as is. |
@Susko3 are you on board with this direction? |
Just to keep this part clear, reverting is mandatory as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a partial fix, as a hovering pen + touch for buttons play style should not work. For that to work, changing OsuTouchInputMapper
is required, and at that point, we might as well go for ppy/osu#31704.
If this was a hack that gets everything in working order in an ugly way, then I would let it pass. But this is a hack that makes things half-working on half the platforms.
if (penDown && device_type == TabletPenDeviceType.Direct) | ||
enqueueInput(new TouchInput(new Input.Touch(TouchSource.PenTouch, position), true)); | ||
else | ||
enqueueInput(new MousePositionAbsoluteInput { Position = position }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this. Mapping pen to touch only on mobile will fix the issue only on mobile.
Having pen input sometimes be touch and sometimes be mouse is hard to reason about, debug and develop for.
After multiple attempts to try and fix ppy/osu#31570, this is one which brings back osu! to how it has been working before.
Now since direct/onscreen pen input is mapped to touch input, the entire pen input /
ISourcedFromPen
API becomes flaky and inconsistent, as not all pen input are sent as anISourcedFromPen
mouse event. For simplicity purposes, I have chosen to revert it completely until later time.When returning back to resurrect this flow, special attention needs to be given into the following intertwined two problems:
PassThroughInputManager.syncReleasedInputs()
should respect mouse input sourced from pen (currently sends release input without such context)PassThroughInputManager.syncReleasedInputs()
should be aware whether mouse button pressed from parent comes from mouse and not other input sources (main reason why it's impossible to make this flow work withOsuTouchInputMapper
)